Skip to content

[WIP] terraform: Consolidate bootstrap resources with module count feature #4822

Closed
rna-afk wants to merge 6 commits intoopenshift:masterfrom
rna-afk:terraform_count_modules
Closed

[WIP] terraform: Consolidate bootstrap resources with module count feature #4822
rna-afk wants to merge 6 commits intoopenshift:masterfrom
rna-afk:terraform_count_modules

Conversation

@rna-afk
Copy link
Copy Markdown
Contributor

@rna-afk rna-afk commented Apr 6, 2021

Terraform 0.13 has a module count feature that can be set to instruct
terraform to create the exact number of copies of the module in the
cloud provider. This feature can be used to control the creation and
deletion of the bootstrap resources needed for cluster.

Adding the count field in all the bootstrap modules of all cloud
providers in order to make it easy to destroy bootstrap after
nodes have been created. This field would also make it easy to
destroy the resources without having to check for the cloud
platform and have a separate destroy logic for each of the platforms.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2021
@rna-afk rna-afk force-pushed the terraform_count_modules branch from e19d2cc to 30457ee Compare April 6, 2021 17:19
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2021
@rna-afk rna-afk force-pushed the terraform_count_modules branch from 30457ee to bc6d971 Compare April 6, 2021 17:21
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2021
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign sdodson after the PR has been reviewed.
You can assign the PR to them by writing /assign @sdodson in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

1 similar comment
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign sdodson after the PR has been reviewed.
You can assign the PR to them by writing /assign @sdodson in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@staebler
Copy link
Copy Markdown
Contributor

staebler commented Apr 6, 2021

This builds on #4729
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2021
Copy link
Copy Markdown
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also want to get rid of the bespoke platform code here.

Comment thread data/data/aws/main.tf Outdated
Comment thread data/data/config.tf Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this variable a bool and rename it to something like bootstrapping.

Comment thread data/data/config.tf Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this statement accurate? Why does it only apply to cloud platforms?

Comment thread pkg/destroy/bootstrap/bootstrap.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this change.

Comment thread data/data/config.tf Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't provide a default. Require that this variable be explicitly set.

@rna-afk rna-afk force-pushed the terraform_count_modules branch from 9a8522e to 78cf8f6 Compare April 7, 2021 00:12
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2021
@rna-afk rna-afk force-pushed the terraform_count_modules branch from 78cf8f6 to 568f27e Compare April 7, 2021 00:40
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2021
@rna-afk rna-afk force-pushed the terraform_count_modules branch from 568f27e to d28e98f Compare April 7, 2021 14:35
Comment thread data/data/aws/main.tf Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move count up to right under source. This keeps it consistent with how count is used for resources and data.

Comment thread data/data/gcp/main.tf Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get rid of gcp_bootstrap_enabled and gcp_bootstrap_lb.

Comment thread data/data/libvirt/main.tf Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get rid of boostrap_dns.

Comment thread data/data/config.tf Outdated
@rna-afk rna-afk force-pushed the terraform_count_modules branch 3 times, most recently from bd9afcd to 37b09aa Compare April 7, 2021 17:29
@rna-afk
Copy link
Copy Markdown
Contributor Author

rna-afk commented Apr 7, 2021

/test e2e-azure
/test e2e-gcp
/test e2e-kubevirt
/test e2e-vsphere

@rna-afk rna-afk force-pushed the terraform_count_modules branch from 37b09aa to 39160d7 Compare April 7, 2021 23:43
@staebler
Copy link
Copy Markdown
Contributor

staebler commented Apr 7, 2021

This looks good to me.

// upload the disk if we don't have an existing template
resource "ovirt_image_transfer" "releaseimage" {
count = length(local.existing_id) == 0 ? 1 : 0
count = length(local.existing_id) == 0 && var.bootstrapping ? 1 : 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to pass in the bootstrapping variable to this module.

@staebler
Copy link
Copy Markdown
Contributor

staebler commented Apr 8, 2021

/test e2e-gcp

@rna-afk rna-afk force-pushed the terraform_count_modules branch 2 times, most recently from 6b81b3d to 50d1081 Compare April 9, 2021 12:31
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2021
@rna-afk rna-afk force-pushed the terraform_count_modules branch from 50d1081 to 03a1a5a Compare April 9, 2021 12:32
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2021
Update terraform-provider-aws to 3.1.0
Update terraform-provider-azure to 2.48.0
Update terraform-provider-google to 3.40.0
Update plugin directory to use new terraform plugin paths. Add environment
variable that sets location of terraform.rc so that local terraform plugins can
be used.

New terraform provider requirements are documented here:

https://www.terraform.io/docs/language/providers/requirements.html
Add terraform.rc and update terraform modules to specify terraform version and
local provider locations.
Add environment variable OPENSHIFT_INSTALL_KEEP_TERRAFORM that will not delete
the installer terraform when set.
@rna-afk rna-afk force-pushed the terraform_count_modules branch from 03a1a5a to 3f4c686 Compare April 9, 2021 13:59
Terraform 0.13 has a module count feature that can be set to instruct
terraform to create the exact number of copies of the module in the
cloud provider. This feature can be used to control the creation and
deletion of the bootstrap resources needed for cluster.

Adding the count field in all the bootstrap modules of all cloud
providers in order to make it easy to destroy bootstrap after
nodes have been created. This field would also make it easy to
destroy the resources without having to check for the cloud
platform and have a separate destroy logic for each of the platforms.
@rna-afk rna-afk force-pushed the terraform_count_modules branch from 3f4c686 to e715485 Compare April 9, 2021 14:03
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@rna-afk: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2021
@openshift-bot
Copy link
Copy Markdown
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 10, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 6, 2021

@rna-afk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-kubevirt 37b09aac3b9c8df7157a16666550d60c28be45ef link /test e2e-kubevirt
ci/prow/e2e-gcp 39160d7 link /test e2e-gcp
ci/prow/tf-fmt e715485 link /test tf-fmt
ci/prow/e2e-openstack e715485 link /test e2e-openstack
ci/prow/e2e-libvirt e715485 link /test e2e-libvirt
ci/prow/e2e-metal-ipi-ovn-ipv6 e715485 link /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-metal-single-node-live-iso e715485 link /test e2e-metal-single-node-live-iso
ci/prow/openstack-manifests e715485 link /test openstack-manifests
ci/prow/e2e-crc e715485 link /test e2e-crc
ci/prow/e2e-ovirt e715485 link /test e2e-ovirt
ci/prow/e2e-aws-upgrade e715485 link /test e2e-aws-upgrade
ci/prow/e2e-aws e715485 link /test e2e-aws
ci/prow/e2e-aws-workers-rhel7 e715485 link /test e2e-aws-workers-rhel7
ci/prow/images e715485 link /test images
ci/prow/okd-verify-codegen e715485 link /test okd-verify-codegen
ci/prow/okd-unit e715485 link /test okd-unit

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 6, 2021

@rna-afk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-kubevirt 37b09aac3b9c8df7157a16666550d60c28be45ef link /test e2e-kubevirt
ci/prow/e2e-gcp 39160d7 link /test e2e-gcp
ci/prow/tf-fmt e715485 link /test tf-fmt
ci/prow/e2e-openstack e715485 link /test e2e-openstack
ci/prow/e2e-libvirt e715485 link /test e2e-libvirt
ci/prow/e2e-metal-ipi-ovn-ipv6 e715485 link /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-metal-single-node-live-iso e715485 link /test e2e-metal-single-node-live-iso
ci/prow/openstack-manifests e715485 link /test openstack-manifests
ci/prow/e2e-crc e715485 link /test e2e-crc
ci/prow/e2e-ovirt e715485 link /test e2e-ovirt
ci/prow/e2e-aws-upgrade e715485 link /test e2e-aws-upgrade
ci/prow/e2e-aws e715485 link /test e2e-aws
ci/prow/e2e-aws-workers-rhel7 e715485 link /test e2e-aws-workers-rhel7
ci/prow/images e715485 link /test images
ci/prow/okd-verify-codegen e715485 link /test okd-verify-codegen
ci/prow/okd-images e715485 link /test okd-images

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rna-afk rna-afk closed this Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants